Exclude groups for sharing #49217

T0mWz · 2024-11-12T08:55:51Z

See also issue #49198

Certain application or configuration options require the use of groups. Think of enabling certain apps for a specific group of the option to restrict autocomplete with sharing for only users in a specific group.
When you make use of these options, it can only result in large groups. When someone accidentally shares data with such a group then, it can have data impact in addition to large system impact.

Example array response which we got in on line; https://github.com/nextcloud/server/blob/master/apps/files_sharing/lib/Controller/ShareesAPIController.php#L200

From this result we will filter the group jupyter as example.
Array will contain a list of groups which will be excluded for sharing purposes.

vi config.php
..
  'blacklisted_groups' =>
  array(
     0 => "admin",
     1 => "jupyter",
     2 => "all_employees",
  ),
..

provokateurin

Please add a test for this

provokateurin · 2024-11-13T09:13:11Z

apps/files_sharing/lib/Controller/ShareesAPIController.php

+		// Get a list of groups to exclude from search results
+		$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);


Suggested change

// Get a list of groups to exclude from search results

$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);

// Get a list of groups to exclude from search results

$sharingDisabledGroups = $this->config->getSystemValue('sharing.disabled_groups', true);

This name is too generic for a system option and blacklist should be avoided (it's better and clearer to use blocklist).

provokateurin · 2024-11-13T09:13:50Z

apps/files_sharing/lib/Controller/ShareesAPIController.php

+		// Get a list of groups to exclude from search results
+		$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);


Suggested change

// Get a list of groups to exclude from search results

$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);

// Get a list of groups to exclude from search results

$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', []);

The default value needs to be an empty array, as otherwise the later code will fail if the config is not set.

provokateurin · 2024-11-13T09:14:10Z

apps/files_sharing/lib/Controller/ShareesAPIController.php

+		    $this->result['groups'] = array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {
+		        return !in_array($group['label'], $blacklisted_groups);
+		    });
+		    // Reindex array to maintain sequential numeric keys
+		    $this->result['groups'] = array_values($this->result['groups']);


Suggested change

$this->result['groups'] = array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {

return !in_array($group['label'], $blacklisted_groups);

});

// Reindex array to maintain sequential numeric keys

$this->result['groups'] = array_values($this->result['groups']);

$this->result['groups'] = array_values(array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {

return !in_array($group['label'], $blacklisted_groups);

}));

provokateurin · 2024-11-13T09:14:26Z

apps/files_sharing/lib/Controller/ShareesAPIController.php

+		    $this->result['exact']['groups'] = array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {
+		        return !in_array($group['label'], $blacklisted_groups);
+		    });
+		    // Reindex array to maintain sequential numeric keys
+		    $this->result['exact']['groups'] = array_values($this->result['exact']['groups']);


Suggested change

$this->result['exact']['groups'] = array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {

return !in_array($group['label'], $blacklisted_groups);

});

// Reindex array to maintain sequential numeric keys

$this->result['exact']['groups'] = array_values($this->result['exact']['groups']);

$this->result['exact']['groups'] = array_values(array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {

return !in_array($group['label'], $blacklisted_groups);

}));

provokateurin · 2024-11-13T09:17:09Z

Also some documentation in https://github.com/nextcloud/documentation about this would be nice as well.

sorbaugh · 2024-11-13T09:17:38Z

@tomwezepoel thanks for the contribution and good luck with the reviews!

provokateurin

Additionally to excluding groups from the search results we should actually prevent creating shares with one of the groups as the recipient. This needs to be implemented in the ShareAPIController.

artonge

Maybe that logic would fit better in lib/private/Collaboration/Collaborators/GroupPlugin.php?
The current solution limits which groups you can search for, but don't limit which you can share with. This is probably the place to add that logic: https://github.com/nextcloud-gmbh/server/blob/1d69ffa48948bbcebc892cc89cd8b551940c58eb/lib/private/Share20/Manager.php#L491-L492
You'll also need to add checkbox in the admin settings. Probably in apps/settings/lib/Settings/Admin/Sharing.php

nfebe

DRY adjustments and possible type guard addition

nfebe · 2024-11-13T09:45:39Z

apps/files_sharing/lib/Controller/ShareesAPIController.php

@@ -197,6 +197,27 @@ public function search(string $search = '', ?string $itemType = null, int $page
 			$result['exact'] = array_merge($this->result['exact'], $result['exact']);
 		}
 		$this->result = array_merge($this->result, $result);
+
+		// Get a list of groups to exclude from search results
+		$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);


Suggested change

$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);

$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);

if (!is_array($blacklisted_groups)) {

$blacklisted_groups = [];

}

Graceful handling if non array is passed? This can be needless if the source if SURE to provide arrays (when probably not configurable)

I think it is fine to assume we get an array, but the default value needs to be an array like I already suggested above.

nfebe · 2024-11-13T09:51:02Z

apps/files_sharing/lib/Controller/ShareesAPIController.php

+		if (isset($this->result['groups'])) {
+		    $this->result['groups'] = array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {
+		        return !in_array($group['label'], $blacklisted_groups);
+		    });
+		    // Reindex array to maintain sequential numeric keys
+		    $this->result['groups'] = array_values($this->result['groups']);
+		}
+
+		if (isset($this->result['exact']['groups'])) {
+		    $this->result['exact']['groups'] = array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {
+		        return !in_array($group['label'], $blacklisted_groups);
+		    });
+		    // Reindex array to maintain sequential numeric keys
+		    $this->result['exact']['groups'] = array_values($this->result['exact']['groups']);
+		}


Suggested change

if (isset($this->result['groups'])) {

$this->result['groups'] = array_filter($this->result['groups'], function($group) use ($blacklisted_groups) {

return !in_array($group['label'], $blacklisted_groups);

});

// Reindex array to maintain sequential numeric keys

$this->result['groups'] = array_values($this->result['groups']);

}

if (isset($this->result['exact']['groups'])) {

$this->result['exact']['groups'] = array_filter($this->result['exact']['groups'], function($group) use ($blacklisted_groups) {

return !in_array($group['label'], $blacklisted_groups);

});

// Reindex array to maintain sequential numeric keys

$this->result['exact']['groups'] = array_values($this->result['exact']['groups']);

}

$filterBlacklistedGroups = function ($groups) use ($blacklisted_groups) {

return array_values(array_filter($groups, function ($group) use ($blacklisted_groups) {

return isset($group['label']) && !in_array($group['label'], $blacklisted_groups);

}));

};

if (isset($this->result['groups'])) {

$this->result['groups'] = $filterBlacklistedGroups($this->result['groups']);

}

if (isset($this->result['exact']['groups'])) {

$this->result['exact']['groups'] = $filterBlacklistedGroups($this->result['exact']['groups']);

}

Since the filter logic is repeated, you can use an inline closure..... or define a function outside this method.

T0mWz · 2024-11-14T09:21:20Z

Thanks for all your feedback, really appreciated. 👍 😃
We will dive into it!

github-actions · 2024-11-27T02:04:01Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Exclude groups for sharing

03e4ba8

provokateurin requested changes Nov 13, 2024

View reviewed changes

artonge reviewed Nov 13, 2024

View reviewed changes

nfebe reviewed Nov 13, 2024

View reviewed changes

github-actions bot added the feedback-requested label Nov 27, 2024

Merge branch 'master' into 49198-exclude_groups_for_sharing

fa1a5ae

redblom mentioned this pull request Dec 17, 2024

[WIP] Block sharing with selected groups. #49897

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude groups for sharing #49217

Exclude groups for sharing #49217

T0mWz commented Nov 12, 2024

provokateurin left a comment

provokateurin Nov 13, 2024

provokateurin Nov 13, 2024

provokateurin Nov 13, 2024

provokateurin Nov 13, 2024

provokateurin commented Nov 13, 2024

sorbaugh commented Nov 13, 2024 •

edited

Loading

provokateurin left a comment

artonge left a comment

nfebe left a comment

nfebe Nov 13, 2024 •

edited

Loading

provokateurin Nov 13, 2024

nfebe Nov 13, 2024 •

edited

Loading

T0mWz commented Nov 14, 2024

github-actions bot commented Nov 27, 2024

		// Get a list of groups to exclude from search results
		$blacklisted_groups = $this->config->getSystemValue('blacklisted_groups', true);

Exclude groups for sharing #49217

Are you sure you want to change the base?

Exclude groups for sharing #49217

Conversation

T0mWz commented Nov 12, 2024

provokateurin left a comment

Choose a reason for hiding this comment

provokateurin Nov 13, 2024

Choose a reason for hiding this comment

provokateurin Nov 13, 2024

Choose a reason for hiding this comment

provokateurin Nov 13, 2024

Choose a reason for hiding this comment

provokateurin Nov 13, 2024

Choose a reason for hiding this comment

provokateurin commented Nov 13, 2024

sorbaugh commented Nov 13, 2024 • edited Loading

provokateurin left a comment

Choose a reason for hiding this comment

artonge left a comment

Choose a reason for hiding this comment

nfebe left a comment

Choose a reason for hiding this comment

nfebe Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

provokateurin Nov 13, 2024

Choose a reason for hiding this comment

nfebe Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

T0mWz commented Nov 14, 2024

github-actions bot commented Nov 27, 2024

sorbaugh commented Nov 13, 2024 •

edited

Loading

nfebe Nov 13, 2024 •

edited

Loading

nfebe Nov 13, 2024 •

edited

Loading